Preserve `Expr`s that have `DefId`s in `ReplaceBodyWithLoop` #73103

ecstatic-morse · 2020-06-07T20:27:22Z

This PR fixes #71820 as well as the last part of #71104 by preserving expressions that are assigned their own DefIds (closures and async blocks) when passing them to rustdoc. This avoids having a DefId without a corresponding HirId.

The first commit in this PR makes -Zunpretty=everybody_loops actually work again, and the subsequent two are miscellaneous cleanup. They should probably get merged regardless of what we end up doing here.

Sample input:

fn foo() -> Box<i32> {
    let x = |a: i64| {
        const FOO: i64 = 1;
    };

    let a = 4;
    Box::new(a)
}

Sample output:

fn foo() -> Box<i32> {
    || -> !
        {
            const FOO: i64 = 1;
            loop  { }
        };
    loop  { }
}

r? @ghost

marmeladema · 2020-06-07T21:10:57Z

Thanks a lot for looking into that! To check if that works, you can modify librustc_privacy/lib.rs as done here https://github.com/rust-lang/rust/pull/72088/files#diff-f7c5ab0a133d5d59a6ce76d8c88ff203 and run rustdoc tests.

ecstatic-morse · 2020-06-07T23:53:41Z

All right, it seems to be working now.

@marmeladema How much ugliness should we be willing to tolerate to resolve #71104? It's an unfortunate architectural mistake that rustdoc needs ReplaceBodyWithLoop in the first place, but with these changes things have gotten pretty heinous. What does fixing #71104 give us? More performance? Memory savings? Clearer code? Presumably whoever reviews this will want to know before deciding whether to sign off on this.

marmeladema · 2020-06-08T11:55:25Z

First thank you for the hard work @ecstatic-morse !

This proposal will solve #71820 (#71104 was more a catch-all issue for all the weird cases I encountered while promoting LocalDefIds).

Without this change public macro 2.0 declared in closures / async blocks / etc cannot be documented, so it might be a blocker for stabilizing macro 2.0?

The same bug also triggered during work in #73101 (but for exactly the same test case) and thus will probably be a blocker for resolving #65983

My personal point of view is that even if this change is not pretty, its not really worse than before, its just a different kind of ugly-ness :P

I think another approach would be to forbid declaration of public macros (or any public / exportable "items" for that matter) in closures and async blocks.

jyn514 · 2020-06-08T13:48:06Z

The same bug also triggered during work in #73101 (but for exactly the same test case) and thus will probably be a blocker for resolving #65983

For context, the exact panic was calling tcx.def_kind(def_id) on a def_id that didn't have a corresponding HirId. I can't fix this in rustdoc because I need to know whether the current DefId is a module (to avoid other bugs), but there's no way to do that without calling def_kind.

ecstatic-morse · 2020-06-09T05:31:08Z

r? @pnkfelix since they reviewed #53002

See the discussion in the comments for motivation. This is just a fixed version of #72088.

pnkfelix · 2020-06-12T14:53:24Z

src/librustc_interface/util.rs

+
+        if let ast::ExprKind::Closure(.., decl, expr, _) = &mut e.kind {
+            // Replacing a closure body and removing its callsites will break inference. Give
+            // all closures the signature `|| -> !` to work around this.


pnkfelix · 2020-06-12T14:59:22Z

@bors r+

bors · 2020-06-12T14:59:24Z

📌 Commit e319f20 has been approved by pnkfelix

@ghost

…op, r=pnkfelix Preserve `Expr`s that have `DefId`s in `ReplaceBodyWithLoop` This PR fixes the last part of rust-lang#71104 by preserving expressions that are assigned their own `DefId`s (closures and `async` blocks) when passing them to `rustdoc`. This avoids having a `DefId` without a corresponding `HirId`. The first commit in this PR makes `-Zunpretty=everybody_loops` actually work again, and the subsequent two are miscellaneous cleanup. They should probably get merged regardless of what we end up doing here. Sample input: ```rust fn foo() -> Box<i32> { let x = |a: i64| { const FOO: i64 = 1; }; let a = 4; Box::new(a) } ``` Sample output: ```rust fn foo() -> Box<i32> { || -> ! { const FOO: i64 = 1; loop { } }; loop { } } ``` r? @ghost

marmeladema · 2020-06-13T09:09:34Z

Fixes #71820

(just adding this comment to automatically close the associated issue when this gets merged)

@ghost

…op, r=pnkfelix Preserve `Expr`s that have `DefId`s in `ReplaceBodyWithLoop` This PR fixes the last part of rust-lang#71104 by preserving expressions that are assigned their own `DefId`s (closures and `async` blocks) when passing them to `rustdoc`. This avoids having a `DefId` without a corresponding `HirId`. The first commit in this PR makes `-Zunpretty=everybody_loops` actually work again, and the subsequent two are miscellaneous cleanup. They should probably get merged regardless of what we end up doing here. Sample input: ```rust fn foo() -> Box<i32> { let x = |a: i64| { const FOO: i64 = 1; }; let a = 4; Box::new(a) } ``` Sample output: ```rust fn foo() -> Box<i32> { || -> ! { const FOO: i64 = 1; loop { } }; loop { } } ``` r? @ghost

Manishearth · 2020-06-15T16:45:57Z

@bors p=1

this is blocking what is now becoming a stack of intra-doc link related patches 😄

@ghost

…op, r=pnkfelix Preserve `Expr`s that have `DefId`s in `ReplaceBodyWithLoop` This PR fixes rust-lang#71820 as well as the last part of rust-lang#71104 by preserving expressions that are assigned their own `DefId`s (closures and `async` blocks) when passing them to `rustdoc`. This avoids having a `DefId` without a corresponding `HirId`. The first commit in this PR makes `-Zunpretty=everybody_loops` actually work again, and the subsequent two are miscellaneous cleanup. They should probably get merged regardless of what we end up doing here. Sample input: ```rust fn foo() -> Box<i32> { let x = |a: i64| { const FOO: i64 = 1; }; let a = 4; Box::new(a) } ``` Sample output: ```rust fn foo() -> Box<i32> { || -> ! { const FOO: i64 = 1; loop { } }; loop { } } ``` r? @ghost

@ghost

…op, r=pnkfelix Preserve `Expr`s that have `DefId`s in `ReplaceBodyWithLoop` This PR fixes rust-lang#71820 as well as the last part of rust-lang#71104 by preserving expressions that are assigned their own `DefId`s (closures and `async` blocks) when passing them to `rustdoc`. This avoids having a `DefId` without a corresponding `HirId`. The first commit in this PR makes `-Zunpretty=everybody_loops` actually work again, and the subsequent two are miscellaneous cleanup. They should probably get merged regardless of what we end up doing here. Sample input: ```rust fn foo() -> Box<i32> { let x = |a: i64| { const FOO: i64 = 1; }; let a = 4; Box::new(a) } ``` Sample output: ```rust fn foo() -> Box<i32> { || -> ! { const FOO: i64 = 1; loop { } }; loop { } } ``` r? @ghost

@ghost

…op, r=pnkfelix Preserve `Expr`s that have `DefId`s in `ReplaceBodyWithLoop` This PR fixes rust-lang#71820 as well as the last part of rust-lang#71104 by preserving expressions that are assigned their own `DefId`s (closures and `async` blocks) when passing them to `rustdoc`. This avoids having a `DefId` without a corresponding `HirId`. The first commit in this PR makes `-Zunpretty=everybody_loops` actually work again, and the subsequent two are miscellaneous cleanup. They should probably get merged regardless of what we end up doing here. Sample input: ```rust fn foo() -> Box<i32> { let x = |a: i64| { const FOO: i64 = 1; }; let a = 4; Box::new(a) } ``` Sample output: ```rust fn foo() -> Box<i32> { || -> ! { const FOO: i64 = 1; loop { } }; loop { } } ``` r? @ghost

ecstatic-morse · 2020-06-17T19:13:50Z

~~I can't build libcore locally with a stage1 compiler at the moment (it hangs), so I don't know when I'll get back to this PR.~~

It was an errant optimize = false left over from #73383.

ecstatic-morse · 2020-06-18T21:42:08Z

@bors r- (bors is having a tough time today)

The problem is with expressions that contain closures, like function calls and references. We'll need to rewrite this pass entirely. Do we need to preserve the number of parent ast::Blocks for each item with a DefId? Is there any alternative to doing this? Can we rebuild the HIR map after this pass runs or something? It seems like we should be able to architect this better instead of piling on more hacks.

jyn514 · 2020-06-18T22:37:45Z

We'll need to rewrite this pass entirely. Is there any alternative to doing this?

What's the purpose of this pass? How is it used currently?

Briefly reading through this, it looks like it

transforms || expr to || { expr }
transforms || {} to || {};
transforms let x = || {}; to || {};
adds loop {} as the last statement in a block

Which of those is creating DefIds with no corresponding HirId? Why add a loop {}?

jyn514 · 2020-06-18T22:50:40Z

These are the relevant commits I found:

$ git log --grep=everybody_loops --oneline
25cc99f privacy: Avoid one more unwrap causing an ICE in rustdoc
aa1e6db Auto merge of #53002 - QuietMisdreavus:brother-may-i-have-some-loops, r=pnkfelix
d6a7a3c add rustdoc test for everybody_loops fix
8df498b more fixes for everybody_loops
f3733a2 make everybody_loops keep item declarations
d2f71bf Auto merge of #44860 - kennytm:fix-44731, r=alexcrichton
16c110f Fix issue #44731.
a7e0018 Auto merge of #43878 - kennytm:fix-43869-put-impl-trait-out-of-the-loop, r=alexcrichton
034cd20 Exclude impl Trait functions from everybody_loops.
0ed03e5 Auto merge of #43348 - kennytm:fix-24658-doc-every-platform, r=alexcrichton
8f935fb Strip out function implementation when documenting.
bf2f84b Add an unstable --xpretty _ option to rustc. Moved flowgraph and everybody_loops options to --xpretty.
41def27 Add --pretty everybody_loops option.

In particular #43348 seems to be the first PR where rustdoc used this pass.

jyn514 · 2020-06-18T22:59:19Z

Ok so the purpose of the pass makes sense to me: rustdoc doesn't need to do item-bodies checking. The issue is that this also strips out new items that appear within the body of the item: macros, trait implementations, and ~~types~~ ignore this, types within a function cannot be seen from any outer scope.

Is there a way to go through the body of the item and keep only the new items? So for example this function:

fn f() {
    macro_rules! m {
        () => {}
    }
    impl MyTrait for MyType {
        fn f() { loop {} }
    }
}

would remain unchanged?

jyn514 · 2020-06-20T02:44:54Z

I think this would be much easier if there were a filter_stmt function on MutVisitor. The goal here is, for any given block:

Keep all statements which are items, possibly nested within other statements.
Remove all other statements.
Add a loop {} statement at the end.

The code in visit_block seems to me like it rewrites most of filter_stmt without factoring it out into something more understandable.

pnkfelix · 2020-06-22T15:57:33Z

I'm personally guessing that PR #73566 is a better way to resolve the rustdoc issues here.

Is there a motivation for this PR beyond that?

Update: Here is a better phrasing of my question: Do the bugs that this fixes also occur when you just feed those source inputs to rustc -Z unpretty=everybody_loops, without running rustdoc?

ecstatic-morse · 2020-06-22T17:22:22Z

I don't think so, although maybe we just bail out earlier when pretty printing, and the first commit was already merged separately.

jyn514 · 2020-06-22T17:32:22Z

@ecstatic-morse #73523 is still waiting on review actually.

I agree that none of the bugs occur when using -Z unpretty=everybody_loops. They only show up when you try to do further analysis on the crate (e.g. by calling def_kind(def_id)).

$ rustc +stage1 -Z unpretty=everybody_loops async_macro.rs  --edition 2018 -A warnings
#![feature(prelude_import)]
// Regression issue for rustdoc ICE encountered in PR #72088.
// edition:2018
#![feature(decl_macro)]
#![no_std]
#[prelude_import]
use core::prelude::v1::*;
#[macro_use]
extern crate core;
#[macro_use]
extern crate compiler_builtins;

fn main() {
    {
        macro m { () => { } }
    }
    loop  { }
}

@eddyb

Don't run `everybody_loops` for rustdoc; instead ignore resolution errors r? @eddyb cc @petrochenkov, @GuillaumeGomez, @Manishearth, @ecstatic-morse, @marmeladema ~~Blocked on rust-lang#73743 Merged. ~~Blocked on crater run.~~ Crater popped up some ICEs ([now fixed](rust-lang#73566 (comment))). See [crater run](https://crater-reports.s3.amazonaws.com/pr-73566/index.html), [ICEs](rust-lang#73566 (comment)). ~~Blocked on rust-lang#74070 so that we don't make typeck_tables_of public when it shouldn't be.~~ Merged. Closes rust-lang#71820, closes rust-lang#71104, closes rust-lang#65863. ## What is the motivation for this change? As seen from a lengthy trail of PRs and issues (rust-lang#73532, rust-lang#73103, rust-lang#71820, rust-lang#71104), `everybody_loops` is causing bugs in rustdoc. The main issue is that it does not preserve the validity of the `DefId` tree, meaning that operations on DefIds may unexpectedly fail when called later. This is blocking intra-doc links (see rust-lang#73101). This PR starts by removing `everybody_loops`, fixing rust-lang#71104 and rust-lang#71820. However, that brings back the bugs seen originally in rust-lang#43348: Since libstd documents items for all platforms, the function bodies sometimes do not type check. Here are the errors from documenting `libstd` with `everybody_loops` disabled and no other changes: ```rust error[E0433]: failed to resolve: could not find `handle` in `sys` --> src/libstd/sys/windows/ext/process.rs:13:27 | 13 | let handle = sys::handle::Handle::new(handle as *mut _); | ^^^^^^ could not find `handle` in `sys` error[E0425]: cannot find function `symlink_inner` in module `sys::fs` --> src/libstd/sys/windows/ext/fs.rs:544:14 | 544 | sys::fs::symlink_inner(src.as_ref(), dst.as_ref(), false) | ^^^^^^^^^^^^^ not found in `sys::fs` error[E0425]: cannot find function `symlink_inner` in module `sys::fs` --> src/libstd/sys/windows/ext/fs.rs:564:14 | 564 | sys::fs::symlink_inner(src.as_ref(), dst.as_ref(), true) | ^^^^^^^^^^^^^ not found in `sys::fs` ``` ## Why does this need changes to `rustc_resolve`? Normally, this could be avoided by simply not calling the `typeck_item_bodies` pass. However, the errors above happen before type checking, in name resolution itself. Since name resolution is intermingled with macro expansion, and rustdoc needs expansion to happen before it knows all items to be documented, there needs to be someway to ignore _resolution_ errors in function bodies. An alternative solution suggested by @petrochenkov was to not run `everybody_loops` on anything containing a nested `DefId`. This would solve some of the immediate issues, but isn't bullet-proof: the following functions still could not be documented if the items in the body failed to resolve: - Functions containing a nested `DefId` (rust-lang#71104) - ~~Functions returning `impl Trait` (rust-lang#43878 These ended up not resolving anyway with this PR. - ~~`const fn`, because `loop {}` in `const fn` is unstable (rust-lang#43636 `const_loop` was just stabilized. This also isn't exactly what rustdoc wants, which is to avoid looking at function bodies in the first place. ## What changes were made? The hack implemented in this PR is to add an option to ignore all resolution errors in function bodies. This is enabled only for rustdoc. Since resolution errors are ignored, the MIR generated will be invalid, as can be seen in the following ICE: ```rust error: internal compiler error: broken MIR in DefId(0:11 ~ doc_cfg[8787]::uses_target_feature[0]) ("return type"): bad type [type error] --> /home/joshua/src/rust/src/test/rustdoc/doc-cfg.rs:51:1 | 51 | / pub unsafe fn uses_target_feature() { 52 | | content::should::be::irrelevant(); 53 | | } | |_^ ``` Fortunately, rustdoc does not need to access MIR in order to generate documentation. Therefore this also removes the call to `analyze()` in `rustdoc::run_core`. This has the side effect of not generating all lints by default. Most lints are safe to ignore (does rustdoc really need to run liveness analysis?) but `missing_docs` in particular is disabled when it should not be. Re-running `missing_docs` specifically does not help, because it causes the typechecking pass to be run, bringing back the errors from rust-lang#24658: ``` error[E0599]: no method named `into_handle` found for struct `sys::unix::pipe::AnonPipe` in the current scope --> src/libstd/sys/windows/ext/process.rs:71:27 | 71 | self.into_inner().into_handle().into_raw() as *mut _ | ^^^^^^^^^^^ method not found in `sys::unix::pipe::AnonPipe` | ``` Because of rust-lang#73743, we only run typeck on demand. So this only causes an issue for functions returning `impl Trait`, which were already special cased by `ReplaceFunctionWithBody`. However, it now considers `async fn f() -> T` to be considered `impl Future<Output = T>`, where before it was considered to have a concrete `T` type. ## How will this affect future changes to rustdoc? - Any new changes to rustdoc will not be able to perform type checking without bringing back resolution errors in function bodies. + As a corollary, any new lints cannot require or perform type checking. In some cases this may require refactoring other parts of the compiler to perform type-checking only on-demand, see for example rust-lang#73743. + As a corollary, rustdoc can never again call `tcx.analysis()` unless this PR is reverted altogether. ## Current status - ~~I am not yet sure how to bring back `missing_docs` without running typeck. @eddyb suggested allowing lints to opt-out of type-checking, which would probably be another rabbit hole.~~ The opt-out was implemented in rust-lang#73743. However, of the rustc lints, now _only_ missing_docs is run and no other lints: rust-lang#73566 (comment). We need a team decision on whether that's an acceptable tradeoff. Note that all rustdoc lints are still run (`intra_doc_link_resolution_failure`, etc). **UPDATE**: This was deemed acceptable in rust-lang#73566 (comment) - ~~The implementation of optional errors in `rustc_resolve` is very brute force, it should probably be moved from `LateResolver` to `Resolver` to avoid duplicating the logic in many places.~~ I'm mostly happy with it now. - This no longer allows errors in `async fn f() -> T`. This caused breakage in 50 crates out of a full crater run, all of which (that I looked at) didn't compile when run with rustc directly. In other words, it used to be that they could not be compiled but could still be documented; now they can't be documented either. This needs a decision from the rustdoc team on whether this is acceptable breakage. **UPDATE**: This was deemed acceptable in rust-lang#73566 (comment) - ~~This makes `fn typeck_tables_of` in `rustc_typeck` public. This is not desired behavior, but needs the changes from rust-lang#74070 in order to be fixed.~~ Reverted.

@eddyb

Don't run `everybody_loops` for rustdoc; instead ignore resolution errors r? @eddyb cc @petrochenkov, @GuillaumeGomez, @Manishearth, @ecstatic-morse, @marmeladema ~~Blocked on rust-lang#73743 Merged. ~~Blocked on crater run.~~ Crater popped up some ICEs ([now fixed](rust-lang#73566 (comment))). See [crater run](https://crater-reports.s3.amazonaws.com/pr-73566/index.html), [ICEs](rust-lang#73566 (comment)). ~~Blocked on rust-lang#74070 so that we don't make typeck_tables_of public when it shouldn't be.~~ Merged. Closes rust-lang#71820, closes rust-lang#71104, closes rust-lang#65863. ## What is the motivation for this change? As seen from a lengthy trail of PRs and issues (rust-lang#73532, rust-lang#73103, rust-lang#71820, rust-lang#71104), `everybody_loops` is causing bugs in rustdoc. The main issue is that it does not preserve the validity of the `DefId` tree, meaning that operations on DefIds may unexpectedly fail when called later. This is blocking intra-doc links (see rust-lang#73101). This PR starts by removing `everybody_loops`, fixing rust-lang#71104 and rust-lang#71820. However, that brings back the bugs seen originally in rust-lang#43348: Since libstd documents items for all platforms, the function bodies sometimes do not type check. Here are the errors from documenting `libstd` with `everybody_loops` disabled and no other changes: ```rust error[E0433]: failed to resolve: could not find `handle` in `sys` --> src/libstd/sys/windows/ext/process.rs:13:27 | 13 | let handle = sys::handle::Handle::new(handle as *mut _); | ^^^^^^ could not find `handle` in `sys` error[E0425]: cannot find function `symlink_inner` in module `sys::fs` --> src/libstd/sys/windows/ext/fs.rs:544:14 | 544 | sys::fs::symlink_inner(src.as_ref(), dst.as_ref(), false) | ^^^^^^^^^^^^^ not found in `sys::fs` error[E0425]: cannot find function `symlink_inner` in module `sys::fs` --> src/libstd/sys/windows/ext/fs.rs:564:14 | 564 | sys::fs::symlink_inner(src.as_ref(), dst.as_ref(), true) | ^^^^^^^^^^^^^ not found in `sys::fs` ``` ## Why does this need changes to `rustc_resolve`? Normally, this could be avoided by simply not calling the `typeck_item_bodies` pass. However, the errors above happen before type checking, in name resolution itself. Since name resolution is intermingled with macro expansion, and rustdoc needs expansion to happen before it knows all items to be documented, there needs to be someway to ignore _resolution_ errors in function bodies. An alternative solution suggested by @petrochenkov was to not run `everybody_loops` on anything containing a nested `DefId`. This would solve some of the immediate issues, but isn't bullet-proof: the following functions still could not be documented if the items in the body failed to resolve: - Functions containing a nested `DefId` (rust-lang#71104) - ~~Functions returning `impl Trait` (rust-lang#43878 These ended up not resolving anyway with this PR. - ~~`const fn`, because `loop {}` in `const fn` is unstable (rust-lang#43636 `const_loop` was just stabilized. This also isn't exactly what rustdoc wants, which is to avoid looking at function bodies in the first place. ## What changes were made? The hack implemented in this PR is to add an option to ignore all resolution errors in function bodies. This is enabled only for rustdoc. Since resolution errors are ignored, the MIR generated will be invalid, as can be seen in the following ICE: ```rust error: internal compiler error: broken MIR in DefId(0:11 ~ doc_cfg[8787]::uses_target_feature[0]) ("return type"): bad type [type error] --> /home/joshua/src/rust/src/test/rustdoc/doc-cfg.rs:51:1 | 51 | / pub unsafe fn uses_target_feature() { 52 | | content::should::be::irrelevant(); 53 | | } | |_^ ``` Fortunately, rustdoc does not need to access MIR in order to generate documentation. Therefore this also removes the call to `analyze()` in `rustdoc::run_core`. This has the side effect of not generating all lints by default. Most lints are safe to ignore (does rustdoc really need to run liveness analysis?) but `missing_docs` in particular is disabled when it should not be. Re-running `missing_docs` specifically does not help, because it causes the typechecking pass to be run, bringing back the errors from rust-lang#24658: ``` error[E0599]: no method named `into_handle` found for struct `sys::unix::pipe::AnonPipe` in the current scope --> src/libstd/sys/windows/ext/process.rs:71:27 | 71 | self.into_inner().into_handle().into_raw() as *mut _ | ^^^^^^^^^^^ method not found in `sys::unix::pipe::AnonPipe` | ``` Because of rust-lang#73743, we only run typeck on demand. So this only causes an issue for functions returning `impl Trait`, which were already special cased by `ReplaceFunctionWithBody`. However, it now considers `async fn f() -> T` to be considered `impl Future<Output = T>`, where before it was considered to have a concrete `T` type. ## How will this affect future changes to rustdoc? - Any new changes to rustdoc will not be able to perform type checking without bringing back resolution errors in function bodies. + As a corollary, any new lints cannot require or perform type checking. In some cases this may require refactoring other parts of the compiler to perform type-checking only on-demand, see for example rust-lang#73743. + As a corollary, rustdoc can never again call `tcx.analysis()` unless this PR is reverted altogether. ## Current status - ~~I am not yet sure how to bring back `missing_docs` without running typeck. @eddyb suggested allowing lints to opt-out of type-checking, which would probably be another rabbit hole.~~ The opt-out was implemented in rust-lang#73743. However, of the rustc lints, now _only_ missing_docs is run and no other lints: rust-lang#73566 (comment). We need a team decision on whether that's an acceptable tradeoff. Note that all rustdoc lints are still run (`intra_doc_link_resolution_failure`, etc). **UPDATE**: This was deemed acceptable in rust-lang#73566 (comment) - ~~The implementation of optional errors in `rustc_resolve` is very brute force, it should probably be moved from `LateResolver` to `Resolver` to avoid duplicating the logic in many places.~~ I'm mostly happy with it now. - This no longer allows errors in `async fn f() -> T`. This caused breakage in 50 crates out of a full crater run, all of which (that I looked at) didn't compile when run with rustc directly. In other words, it used to be that they could not be compiled but could still be documented; now they can't be documented either. This needs a decision from the rustdoc team on whether this is acceptable breakage. **UPDATE**: This was deemed acceptable in rust-lang#73566 (comment) - ~~This makes `fn typeck_tables_of` in `rustc_typeck` public. This is not desired behavior, but needs the changes from rust-lang#74070 in order to be fixed.~~ Reverted.

@eddyb

Don't run `everybody_loops` for rustdoc; instead ignore resolution errors r? @eddyb cc @petrochenkov, @GuillaumeGomez, @Manishearth, @ecstatic-morse, @marmeladema ~~Blocked on rust-lang#73743 Merged. ~~Blocked on crater run.~~ Crater popped up some ICEs ([now fixed](rust-lang#73566 (comment))). See [crater run](https://crater-reports.s3.amazonaws.com/pr-73566/index.html), [ICEs](rust-lang#73566 (comment)). ~~Blocked on rust-lang#74070 so that we don't make typeck_tables_of public when it shouldn't be.~~ Merged. Closes rust-lang#71820, closes rust-lang#71104, closes rust-lang#65863. ## What is the motivation for this change? As seen from a lengthy trail of PRs and issues (rust-lang#73532, rust-lang#73103, rust-lang#71820, rust-lang#71104), `everybody_loops` is causing bugs in rustdoc. The main issue is that it does not preserve the validity of the `DefId` tree, meaning that operations on DefIds may unexpectedly fail when called later. This is blocking intra-doc links (see rust-lang#73101). This PR starts by removing `everybody_loops`, fixing rust-lang#71104 and rust-lang#71820. However, that brings back the bugs seen originally in rust-lang#43348: Since libstd documents items for all platforms, the function bodies sometimes do not type check. Here are the errors from documenting `libstd` with `everybody_loops` disabled and no other changes: ```rust error[E0433]: failed to resolve: could not find `handle` in `sys` --> src/libstd/sys/windows/ext/process.rs:13:27 | 13 | let handle = sys::handle::Handle::new(handle as *mut _); | ^^^^^^ could not find `handle` in `sys` error[E0425]: cannot find function `symlink_inner` in module `sys::fs` --> src/libstd/sys/windows/ext/fs.rs:544:14 | 544 | sys::fs::symlink_inner(src.as_ref(), dst.as_ref(), false) | ^^^^^^^^^^^^^ not found in `sys::fs` error[E0425]: cannot find function `symlink_inner` in module `sys::fs` --> src/libstd/sys/windows/ext/fs.rs:564:14 | 564 | sys::fs::symlink_inner(src.as_ref(), dst.as_ref(), true) | ^^^^^^^^^^^^^ not found in `sys::fs` ``` ## Why does this need changes to `rustc_resolve`? Normally, this could be avoided by simply not calling the `typeck_item_bodies` pass. However, the errors above happen before type checking, in name resolution itself. Since name resolution is intermingled with macro expansion, and rustdoc needs expansion to happen before it knows all items to be documented, there needs to be someway to ignore _resolution_ errors in function bodies. An alternative solution suggested by @petrochenkov was to not run `everybody_loops` on anything containing a nested `DefId`. This would solve some of the immediate issues, but isn't bullet-proof: the following functions still could not be documented if the items in the body failed to resolve: - Functions containing a nested `DefId` (rust-lang#71104) - ~~Functions returning `impl Trait` (rust-lang#43878 These ended up not resolving anyway with this PR. - ~~`const fn`, because `loop {}` in `const fn` is unstable (rust-lang#43636 `const_loop` was just stabilized. This also isn't exactly what rustdoc wants, which is to avoid looking at function bodies in the first place. ## What changes were made? The hack implemented in this PR is to add an option to ignore all resolution errors in function bodies. This is enabled only for rustdoc. Since resolution errors are ignored, the MIR generated will be invalid, as can be seen in the following ICE: ```rust error: internal compiler error: broken MIR in DefId(0:11 ~ doc_cfg[8787]::uses_target_feature[0]) ("return type"): bad type [type error] --> /home/joshua/src/rust/src/test/rustdoc/doc-cfg.rs:51:1 | 51 | / pub unsafe fn uses_target_feature() { 52 | | content::should::be::irrelevant(); 53 | | } | |_^ ``` Fortunately, rustdoc does not need to access MIR in order to generate documentation. Therefore this also removes the call to `analyze()` in `rustdoc::run_core`. This has the side effect of not generating all lints by default. Most lints are safe to ignore (does rustdoc really need to run liveness analysis?) but `missing_docs` in particular is disabled when it should not be. Re-running `missing_docs` specifically does not help, because it causes the typechecking pass to be run, bringing back the errors from rust-lang#24658: ``` error[E0599]: no method named `into_handle` found for struct `sys::unix::pipe::AnonPipe` in the current scope --> src/libstd/sys/windows/ext/process.rs:71:27 | 71 | self.into_inner().into_handle().into_raw() as *mut _ | ^^^^^^^^^^^ method not found in `sys::unix::pipe::AnonPipe` | ``` Because of rust-lang#73743, we only run typeck on demand. So this only causes an issue for functions returning `impl Trait`, which were already special cased by `ReplaceFunctionWithBody`. However, it now considers `async fn f() -> T` to be considered `impl Future<Output = T>`, where before it was considered to have a concrete `T` type. ## How will this affect future changes to rustdoc? - Any new changes to rustdoc will not be able to perform type checking without bringing back resolution errors in function bodies. + As a corollary, any new lints cannot require or perform type checking. In some cases this may require refactoring other parts of the compiler to perform type-checking only on-demand, see for example rust-lang#73743. + As a corollary, rustdoc can never again call `tcx.analysis()` unless this PR is reverted altogether. ## Current status - ~~I am not yet sure how to bring back `missing_docs` without running typeck. @eddyb suggested allowing lints to opt-out of type-checking, which would probably be another rabbit hole.~~ The opt-out was implemented in rust-lang#73743. However, of the rustc lints, now _only_ missing_docs is run and no other lints: rust-lang#73566 (comment). We need a team decision on whether that's an acceptable tradeoff. Note that all rustdoc lints are still run (`intra_doc_link_resolution_failure`, etc). **UPDATE**: This was deemed acceptable in rust-lang#73566 (comment) - ~~The implementation of optional errors in `rustc_resolve` is very brute force, it should probably be moved from `LateResolver` to `Resolver` to avoid duplicating the logic in many places.~~ I'm mostly happy with it now. - This no longer allows errors in `async fn f() -> T`. This caused breakage in 50 crates out of a full crater run, all of which (that I looked at) didn't compile when run with rustc directly. In other words, it used to be that they could not be compiled but could still be documented; now they can't be documented either. This needs a decision from the rustdoc team on whether this is acceptable breakage. **UPDATE**: This was deemed acceptable in rust-lang#73566 (comment) - ~~This makes `fn typeck_tables_of` in `rustc_typeck` public. This is not desired behavior, but needs the changes from rust-lang#74070 in order to be fixed.~~ Reverted.

@eddyb

Don't run `everybody_loops` for rustdoc; instead ignore resolution errors r? @eddyb cc @petrochenkov, @GuillaumeGomez, @Manishearth, @ecstatic-morse, @marmeladema ~~Blocked on rust-lang#73743 Merged. ~~Blocked on crater run.~~ Crater popped up some ICEs ([now fixed](rust-lang#73566 (comment))). See [crater run](https://crater-reports.s3.amazonaws.com/pr-73566/index.html), [ICEs](rust-lang#73566 (comment)). ~~Blocked on rust-lang#74070 so that we don't make typeck_tables_of public when it shouldn't be.~~ Merged. Closes rust-lang#71820, closes rust-lang#71104, closes rust-lang#65863. ## What is the motivation for this change? As seen from a lengthy trail of PRs and issues (rust-lang#73532, rust-lang#73103, rust-lang#71820, rust-lang#71104), `everybody_loops` is causing bugs in rustdoc. The main issue is that it does not preserve the validity of the `DefId` tree, meaning that operations on DefIds may unexpectedly fail when called later. This is blocking intra-doc links (see rust-lang#73101). This PR starts by removing `everybody_loops`, fixing rust-lang#71104 and rust-lang#71820. However, that brings back the bugs seen originally in rust-lang#43348: Since libstd documents items for all platforms, the function bodies sometimes do not type check. Here are the errors from documenting `libstd` with `everybody_loops` disabled and no other changes: ```rust error[E0433]: failed to resolve: could not find `handle` in `sys` --> src/libstd/sys/windows/ext/process.rs:13:27 | 13 | let handle = sys::handle::Handle::new(handle as *mut _); | ^^^^^^ could not find `handle` in `sys` error[E0425]: cannot find function `symlink_inner` in module `sys::fs` --> src/libstd/sys/windows/ext/fs.rs:544:14 | 544 | sys::fs::symlink_inner(src.as_ref(), dst.as_ref(), false) | ^^^^^^^^^^^^^ not found in `sys::fs` error[E0425]: cannot find function `symlink_inner` in module `sys::fs` --> src/libstd/sys/windows/ext/fs.rs:564:14 | 564 | sys::fs::symlink_inner(src.as_ref(), dst.as_ref(), true) | ^^^^^^^^^^^^^ not found in `sys::fs` ``` ## Why does this need changes to `rustc_resolve`? Normally, this could be avoided by simply not calling the `typeck_item_bodies` pass. However, the errors above happen before type checking, in name resolution itself. Since name resolution is intermingled with macro expansion, and rustdoc needs expansion to happen before it knows all items to be documented, there needs to be someway to ignore _resolution_ errors in function bodies. An alternative solution suggested by @petrochenkov was to not run `everybody_loops` on anything containing a nested `DefId`. This would solve some of the immediate issues, but isn't bullet-proof: the following functions still could not be documented if the items in the body failed to resolve: - Functions containing a nested `DefId` (rust-lang#71104) - ~~Functions returning `impl Trait` (rust-lang#43878 These ended up not resolving anyway with this PR. - ~~`const fn`, because `loop {}` in `const fn` is unstable (rust-lang#43636 `const_loop` was just stabilized. This also isn't exactly what rustdoc wants, which is to avoid looking at function bodies in the first place. ## What changes were made? The hack implemented in this PR is to add an option to ignore all resolution errors in function bodies. This is enabled only for rustdoc. Since resolution errors are ignored, the MIR generated will be invalid, as can be seen in the following ICE: ```rust error: internal compiler error: broken MIR in DefId(0:11 ~ doc_cfg[8787]::uses_target_feature[0]) ("return type"): bad type [type error] --> /home/joshua/src/rust/src/test/rustdoc/doc-cfg.rs:51:1 | 51 | / pub unsafe fn uses_target_feature() { 52 | | content::should::be::irrelevant(); 53 | | } | |_^ ``` Fortunately, rustdoc does not need to access MIR in order to generate documentation. Therefore this also removes the call to `analyze()` in `rustdoc::run_core`. This has the side effect of not generating all lints by default. Most lints are safe to ignore (does rustdoc really need to run liveness analysis?) but `missing_docs` in particular is disabled when it should not be. Re-running `missing_docs` specifically does not help, because it causes the typechecking pass to be run, bringing back the errors from rust-lang#24658: ``` error[E0599]: no method named `into_handle` found for struct `sys::unix::pipe::AnonPipe` in the current scope --> src/libstd/sys/windows/ext/process.rs:71:27 | 71 | self.into_inner().into_handle().into_raw() as *mut _ | ^^^^^^^^^^^ method not found in `sys::unix::pipe::AnonPipe` | ``` Because of rust-lang#73743, we only run typeck on demand. So this only causes an issue for functions returning `impl Trait`, which were already special cased by `ReplaceFunctionWithBody`. However, it now considers `async fn f() -> T` to be considered `impl Future<Output = T>`, where before it was considered to have a concrete `T` type. ## How will this affect future changes to rustdoc? - Any new changes to rustdoc will not be able to perform type checking without bringing back resolution errors in function bodies. + As a corollary, any new lints cannot require or perform type checking. In some cases this may require refactoring other parts of the compiler to perform type-checking only on-demand, see for example rust-lang#73743. + As a corollary, rustdoc can never again call `tcx.analysis()` unless this PR is reverted altogether. ## Current status - ~~I am not yet sure how to bring back `missing_docs` without running typeck. @eddyb suggested allowing lints to opt-out of type-checking, which would probably be another rabbit hole.~~ The opt-out was implemented in rust-lang#73743. However, of the rustc lints, now _only_ missing_docs is run and no other lints: rust-lang#73566 (comment). We need a team decision on whether that's an acceptable tradeoff. Note that all rustdoc lints are still run (`intra_doc_link_resolution_failure`, etc). **UPDATE**: This was deemed acceptable in rust-lang#73566 (comment) - ~~The implementation of optional errors in `rustc_resolve` is very brute force, it should probably be moved from `LateResolver` to `Resolver` to avoid duplicating the logic in many places.~~ I'm mostly happy with it now. - This no longer allows errors in `async fn f() -> T`. This caused breakage in 50 crates out of a full crater run, all of which (that I looked at) didn't compile when run with rustc directly. In other words, it used to be that they could not be compiled but could still be documented; now they can't be documented either. This needs a decision from the rustdoc team on whether this is acceptable breakage. **UPDATE**: This was deemed acceptable in rust-lang#73566 (comment) - ~~This makes `fn typeck_tables_of` in `rustc_typeck` public. This is not desired behavior, but needs the changes from rust-lang#74070 in order to be fixed.~~ Reverted.

…anishearth Resolve items for cross-crate imports relative to the original module ~~Blocked on rust-lang#73103 and rust-lang#73566 Closes rust-lang#65983. I tested on the following code (as mentioned in rust-lang#65983 (comment)): ``` pub use rand::Rng; ``` and rustdoc generated the following link: https://rust-random.github.io/rand/rand_core/trait.RngCore.html

ecstatic-morse force-pushed the replace-body-with-loop branch from c638690 to 721048f Compare June 7, 2020 21:05

ecstatic-morse force-pushed the replace-body-with-loop branch 2 times, most recently from 35ede76 to e319f20 Compare June 7, 2020 23:14

rust-highfive assigned pnkfelix Jun 9, 2020

ecstatic-morse marked this pull request as ready for review June 9, 2020 15:52

jyn514 mentioned this pull request Jun 10, 2020

Resolve items for cross-crate imports relative to the original module #73101

Merged

ecstatic-morse mentioned this pull request Jun 11, 2020

Do not remove closures during ReplaceBodyWithLoop pass #72088

Closed

pnkfelix reviewed Jun 12, 2020

View reviewed changes

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jun 12, 2020

Dylan-DPC-zz mentioned this pull request Jun 12, 2020

Rollup of 6 pull requests #73289

Closed

Dylan-DPC-zz mentioned this pull request Jun 13, 2020

Rollup of 5 pull requests #73325

Closed

Manishearth mentioned this pull request Jun 16, 2020

Rollup of 14 pull requests #73392

Closed

Manishearth mentioned this pull request Jun 16, 2020

Rollup of 14 pull requests #73393

Closed

Manishearth mentioned this pull request Jun 16, 2020

Rollup of 10 pull requests #73408

Closed

pnkfelix added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 17, 2020

ecstatic-morse closed this Jun 18, 2020

ecstatic-morse reopened this Jun 18, 2020

jyn514 mentioned this pull request Jun 20, 2020

Fix -Z unpretty=everybody_loops #73523

Merged

This was referenced Jun 20, 2020

Don't generate fake IDs in ReplaceBodyWithLoop #73532

Closed

Don't run everybody_loops for rustdoc; instead ignore resolution errors #73566

Merged

ecstatic-morse closed this Jun 22, 2020

jyn514 mentioned this pull request Jun 29, 2020

Some NodeId/LocalDefId don't have a corresponding HirId #71104

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve `Expr`s that have `DefId`s in `ReplaceBodyWithLoop` #73103

Preserve `Expr`s that have `DefId`s in `ReplaceBodyWithLoop` #73103

ecstatic-morse commented Jun 7, 2020 •

edited

Loading

marmeladema commented Jun 7, 2020

ecstatic-morse commented Jun 7, 2020 •

edited

Loading

marmeladema commented Jun 8, 2020

jyn514 commented Jun 8, 2020

ecstatic-morse commented Jun 9, 2020 •

edited

Loading

pnkfelix Jun 12, 2020

pnkfelix commented Jun 12, 2020

bors commented Jun 12, 2020

marmeladema commented Jun 13, 2020 •

edited

Loading

Manishearth commented Jun 15, 2020

ecstatic-morse commented Jun 17, 2020 •

edited

Loading

ecstatic-morse commented Jun 18, 2020

jyn514 commented Jun 18, 2020 •

edited

Loading

jyn514 commented Jun 18, 2020 •

edited

Loading

jyn514 commented Jun 18, 2020 •

edited

Loading

jyn514 commented Jun 20, 2020 •

edited

Loading

pnkfelix commented Jun 22, 2020 •

edited

Loading

ecstatic-morse commented Jun 22, 2020

jyn514 commented Jun 22, 2020

Preserve Exprs that have DefIds in ReplaceBodyWithLoop #73103

Preserve Exprs that have DefIds in ReplaceBodyWithLoop #73103

Conversation

ecstatic-morse commented Jun 7, 2020 • edited Loading

marmeladema commented Jun 7, 2020

ecstatic-morse commented Jun 7, 2020 • edited Loading

marmeladema commented Jun 8, 2020

jyn514 commented Jun 8, 2020

ecstatic-morse commented Jun 9, 2020 • edited Loading

pnkfelix Jun 12, 2020

Choose a reason for hiding this comment

pnkfelix commented Jun 12, 2020

bors commented Jun 12, 2020

marmeladema commented Jun 13, 2020 • edited Loading

Manishearth commented Jun 15, 2020

ecstatic-morse commented Jun 17, 2020 • edited Loading

ecstatic-morse commented Jun 18, 2020

jyn514 commented Jun 18, 2020 • edited Loading

jyn514 commented Jun 18, 2020 • edited Loading

jyn514 commented Jun 18, 2020 • edited Loading

jyn514 commented Jun 20, 2020 • edited Loading

pnkfelix commented Jun 22, 2020 • edited Loading

ecstatic-morse commented Jun 22, 2020

jyn514 commented Jun 22, 2020

Preserve `Expr`s that have `DefId`s in `ReplaceBodyWithLoop` #73103

Preserve `Expr`s that have `DefId`s in `ReplaceBodyWithLoop` #73103

ecstatic-morse commented Jun 7, 2020 •

edited

Loading

ecstatic-morse commented Jun 7, 2020 •

edited

Loading

ecstatic-morse commented Jun 9, 2020 •

edited

Loading

marmeladema commented Jun 13, 2020 •

edited

Loading

ecstatic-morse commented Jun 17, 2020 •

edited

Loading

jyn514 commented Jun 18, 2020 •

edited

Loading

jyn514 commented Jun 18, 2020 •

edited

Loading

jyn514 commented Jun 18, 2020 •

edited

Loading

jyn514 commented Jun 20, 2020 •

edited

Loading

pnkfelix commented Jun 22, 2020 •

edited

Loading